Add --add-ignore for adding ruff:ignore comments#26346
Conversation
|
|
As part of this PR, I'm checking how well Then there are 923 new RUF100 diagnostics from a variety of rules that seem not to be suppressible. One example is RUF101: https://play.ruff.rs/45d0e92f-fe87-437e-af87-9a83b099ff5b. Another is PLR2044: https://play.ruff.rs/9bcf9da7-4a50-494b-b6d0-cda53188b876. Another is SIM109: https://play.ruff.rs/922b0cf5-b97e-4c9f-8bdc-e300283f35fe. We can also introduce syntax errors, mostly via UP031 from what I've seen so far: ❯ cat try.py
print("A long string \ # noqa: UP031
with a continuation on line %d" % 2)
❯ python try.py
File "try.py", line 1
print("A long string \ # noqa: UP031
^
SyntaxError: unterminated string literal (detected at line 1)There are only 10 of these, but we can also disrupt (our detection of?) script metadata and trigger INP001: # /// script # noqa: CPY001
# requires-python = ">=3.11"
# dependencies = [
# "uv==0.11.13",I'll plan to open a separate issue or issues for these, but for now I'll just try to make sure |
3b9f7c7 to
ac10c98
Compare
Summary -- This PR adds the `--add-ignore` flag as an alias to `--add-noqa`. In preview, this adds `ruff:ignore` comments with human-readable names instead of `noqa` comments with codes. I initially drafted this PR assuming that `--add-ignore` would be a separate flag that only functioned in preview mode, while `--add-noqa` would be available in both preview and stable, but after talking with Micha realized that we probably want to go ahead and push preview users toward `--add-ignore` entirely rather than giving them the choice. I think the one downside of the alias approach is that `--add-noqa` still exists in preview but will still add `ruff:ignore` comments. In cases where an existing `noqa` or `ruff:ignore` comment is present, I opted to preserve the existing comment and to append the new comment. So something like this: ```py import math # noqa: RUF100 ``` becomes: ```py import math # noqa: RUF100 # ruff:ignore[unused-import] ``` instead of also converting the existing `noqa` comment. I think we can defer to one of our planned lint rules for the conversion (and joining) instead. As also shown in this example, the flag adds human-readable names rather than rule codes and prefers the same placement as a `noqa` comment, at the end of the line. The one exception to this is an existing `ruff:ignore` comment on its own line. In that case, we will reuse and extend the existing comment instead of adding an additional trailing comment, which I think is a pretty nice balance. Test Plan -- Many new CLI tests exercising the various cases Codex and I could come up with. As the server test shows, this actually applies to the LSP as well, where the `Disable for this line` action now also inserts a `ruff:ignore` comment with a rule name instead of a `noqa` comment.
Hmmm, I don't remember this conversation. Given recent feedback. I'm leaning towards preserving |
|
Maybe I misremembered, but it was a couple of weeks ago. Anyway, happy to preserve both flags. |
|
Sorry, to be clear. I'm not saying you made this up. I just don't remember it, which doesn't mean that I didn't say something in that direction |
MichaReiser
left a comment
There was a problem hiding this comment.
This looks good to me as a first version. We can iterate on it based on user feedback. I do suggest not making --add-noqa an alias of --add-ignore. Instead, --add-noqa adds noqa comments, and --add-ignore uses ruff:ignore. Making it an alias is a problem for users who want to continue using noqa codes because they now loose this functionality.
I do suggest replacing the noqa naming in add_noqa with suppression and rename the corresponding function and module.
Unfortunately, my codex interrogation on noqa and ruff:ignore semantics uncovered a few more bugs in ruff:ignore. I don't think they need to block this PR, but we should look into them.
if True:
first = 1 # ruff:ignore[indentation-with-invalid-multiple]
second = 2 # ruff:ignore[indentation-with-invalid-multiple]This does not work, but the corresponding noqa: E111 works
(I must admit, these specific ignore comments look rather verbose)
The same for
import logging
logger = logging.getLogger(__name__)
name = "world"
logger.error(
f"""Hello {
name
}""", # noqa: G004
)
logger.error(
f"""Hello {
name
}""", # ruff:ignore[logging-f-string]
)and
#!/usr/bin/env python3
from __future__ import annotations # ruff:ignore[undocumented-public-module]This also does not work with ruff:ignore, but works with noqa
value = 1 # noqa: F401 # ruff:ignore[unused-noqa]| suppression_kind, | ||
| ); | ||
| build_noqa_edits_by_diagnostic(comments, locator, line_ending, None) | ||
| build_noqa_edits_by_diagnostic(comments, locator, line_ending, None, suppression_kind) |
There was a problem hiding this comment.
should this be renamed to say build_suppression_edits_by_diagnostic. We may want to do so in the entire module, including the module itself.
There was a problem hiding this comment.
Good idea. I'm renaming the relevant noqa references to suppression, including the module.
There was a problem hiding this comment.
Hmm, the ruff:ignore comments are actually already in a module called suppression. As a quick fix, I was going to rename that module to suppression_comments, but I'm wondering if we might want a different structure. Something like a top-level suppression with nested noqa and ignore modules could work, if we don't mind lumping enable/disable under the ignore name. A lot of the code in the noqa module is still specific to noqa comments, but these shared functions could live in the parent suppression module.
I think I'll stick with the function renames here and then follow up on the modules if you think that sounds like a good idea.
| match directive { | ||
| None => { | ||
| match (directive, suppression_kind) { | ||
| (Some(ExistingDirective::Noqa(codes)), SuppressionKind::Noqa) => { |
There was a problem hiding this comment.
It may be worth adding some inline comments here (what are these different arms)
- Add code to an existing noqa
- Add code to an existing ruff:ignore
- No existing comment or "wrong" comment
| ) -> (TextRange, bool) { | ||
| let prefix = locator.slice(TextRange::new(line_range.start(), directive_start)); | ||
| if prefix.trim_whitespace().is_empty() { | ||
| (TextRange::new(directive_start, line_range.end()), true) |
There was a problem hiding this comment.
Can we document what this method returns. What's the meaning of the second bool
| // Prefer extending an existing `ruff:ignore` over an existing `noqa` directive. | ||
| let directive = suppressions | ||
| .find_applicable_ignore(message) | ||
| .map(ExistingDirective::Ignore) | ||
| .or(existing_noqa); |
There was a problem hiding this comment.
Reading the code below. It seems to be that we never reuse an existing suppression comment if it doesn't match the requested suppression style. This comment sort of suggests the opposite to me. Or is it, if we have two suppression comments on the same line, find the first that matches the expected suppression style?
There was a problem hiding this comment.
Ah yeah, I'll rephrase this. I was trying to say something more like "Prioritize an existing ruff:ignore over an existing noqa directive on the same line." For example, we want to extend ruff:ignore in this case instead of appending another ruff:ignore just because we found noqa first:
import math # noqa: F401 # ruff:ignore[unused-import]Actually, I think this logic will have to change when the flag is no longer an alias, though. In the current state of the PR, it's assumed that if a ruff:ignore comment exists and is active, then preview is obviously enabled and --add-noqa/--add-ignore is thus adding an ignore comment. As you said, what really needs to be prioritized is the requested suppression style.
Thanks! Yeah, I had done a similar investigation and spun those bug fixes off into separate (draft) PRs to avoid blocking this one. It looks like we mostly found the same things.
This should be covered by #26412.
Codex says this one is "partially" fixed by #26412 by adjusting the comment placement for
This case should be addressed by #26373.
This should also be covered by #26412. |
|
I moved the alias to a separate flag that conflicts with I also moved most of the tests to the |
Summary
This PR adds the new
--add-ignoreflag. In preview, this addsruff:ignorecomments with human-readable names instead ofnoqacomments with codes. Without preview enabled, using the flag emits an error.--add-noqaitself is still available in both preview and stable mode.In cases where an existing
noqaorruff:ignorecomment is present, I opted to preserve theexisting comment and to append the new comment. So something like this:
becomes:
instead of also converting the existing
noqacomment. I think we can defer to one of our plannedlint rules for the conversion (and joining) instead.
As also shown in this example, the flag adds human-readable names rather than rule codes and prefers
the same placement as a
noqacomment, at the end of the line. The one exception to this is anexisting
ruff:ignorecomment on its own line. In that case, we will reuse and extend the existingcomment instead of adding an additional trailing comment, which I think is a pretty nice balance.
Test Plan
Many new tests exercising the various cases Codex and I could come up with. As the server test
shows, this actually applies to the LSP as well, where the
Disable for this lineaction now alsoinserts a
ruff:ignorecomment with a rule name instead of anoqacomment.Ecosystem check
I also ran some tests against ecosystem projects as described in my comment below. These revealed some additional discrepancies between
ruff:ignoreandnoqa(and some bugs innoqa/--add-noqaitself), but I think it makes sense to leave these as follow-ups. I've already opened (rough) drafts of them in:ruff:ignoreedge cases after a shebang #26373--add-noqaand--add-ignore#26409ruff:ignorefixes #26412We could land the shebang fixes separately, but the other two are stacked on this PR. With all three of these applied, the ecosystem check showed parity with
--add-noqa.This also seems like an interesting check to add to our ecosystem analysis in general.